Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[libc++] Stop copying headers to the build directory #115380

Open
wants to merge 6 commits into
base: users/arichardson/spr/main.libc-stop-copying-headers-to-the-build-directory
Choose a base branch
from

Conversation

arichardson
Copy link
Member

This was needed before #115077
since the compiler-rt test build made assumptions about the build
layout of libc++ and libc++abi, but now they link against a local
installation of these libraries so we no longer need this workaround.

Created using spr 1.3.6-beta.1
@arichardson arichardson requested a review from a team as a code owner November 7, 2024 22:21
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Nov 7, 2024
@llvmbot
Copy link

llvmbot commented Nov 7, 2024

@llvm/pr-subscribers-lldb

@llvm/pr-subscribers-libcxx

Author: Alexander Richardson (arichardson)

Changes

This was needed before #115077
since the compiler-rt test build made assumptions about the build
layout of libc++ and libc++abi, but now they link against a local
installation of these libraries so we no longer need this workaround.


Full diff: https://github.com/llvm/llvm-project/pull/115380.diff

1 Files Affected:

  • (modified) libcxx/CMakeLists.txt (+4-7)
diff --git a/libcxx/CMakeLists.txt b/libcxx/CMakeLists.txt
index d699135774ee0b..04596fccdfc923 100644
--- a/libcxx/CMakeLists.txt
+++ b/libcxx/CMakeLists.txt
@@ -414,15 +414,16 @@ set(LIBCXX_INSTALL_MODULES_DIR "share/libc++/v1" CACHE STRING
 set(LIBCXX_SHARED_OUTPUT_NAME "c++" CACHE STRING "Output name for the shared libc++ runtime library.")
 set(LIBCXX_STATIC_OUTPUT_NAME "c++" CACHE STRING "Output name for the static libc++ runtime library.")
 
+set(LIBCXX_GENERATED_INCLUDE_DIR "${LIBCXX_BINARY_DIR}/include/c++/v1")
+set(LIBCXX_GENERATED_MODULE_DIR "${LIBCXX_BINARY_DIR}/modules/c++/v1")
+
 if(LLVM_ENABLE_PER_TARGET_RUNTIME_DIR AND NOT APPLE)
   set(LIBCXX_TARGET_SUBDIR ${LLVM_DEFAULT_TARGET_TRIPLE})
   if(LIBCXX_LIBDIR_SUBDIR)
     string(APPEND LIBCXX_TARGET_SUBDIR /${LIBCXX_LIBDIR_SUBDIR})
   endif()
   set(LIBCXX_LIBRARY_DIR ${LLVM_LIBRARY_OUTPUT_INTDIR}/${LIBCXX_TARGET_SUBDIR})
-  set(LIBCXX_GENERATED_INCLUDE_DIR "${LLVM_BINARY_DIR}/include/c++/v1")
-  set(LIBCXX_GENERATED_MODULE_DIR "${LLVM_BINARY_DIR}/modules/c++/v1")
-  set(LIBCXX_GENERATED_INCLUDE_TARGET_DIR "${LLVM_BINARY_DIR}/include/${LIBCXX_TARGET_SUBDIR}/c++/v1")
+  set(LIBCXX_GENERATED_INCLUDE_TARGET_DIR "${LIBCXX_BINARY_DIR}/include/${LIBCXX_TARGET_SUBDIR}/c++/v1")
   set(LIBCXX_INSTALL_LIBRARY_DIR lib${LLVM_LIBDIR_SUFFIX}/${LIBCXX_TARGET_SUBDIR} CACHE STRING
       "Path where built libc++ libraries should be installed.")
   set(LIBCXX_INSTALL_INCLUDE_TARGET_DIR "${CMAKE_INSTALL_INCLUDEDIR}/${LIBCXX_TARGET_SUBDIR}/c++/v1" CACHE STRING
@@ -431,12 +432,8 @@ if(LLVM_ENABLE_PER_TARGET_RUNTIME_DIR AND NOT APPLE)
 else()
   if(LLVM_LIBRARY_OUTPUT_INTDIR)
     set(LIBCXX_LIBRARY_DIR ${LLVM_LIBRARY_OUTPUT_INTDIR})
-    set(LIBCXX_GENERATED_INCLUDE_DIR "${LLVM_BINARY_DIR}/include/c++/v1")
-    set(LIBCXX_GENERATED_MODULE_DIR "${LLVM_BINARY_DIR}/modules/c++/v1")
   else()
     set(LIBCXX_LIBRARY_DIR ${CMAKE_BINARY_DIR}/lib${LIBCXX_LIBDIR_SUFFIX})
-    set(LIBCXX_GENERATED_INCLUDE_DIR "${CMAKE_BINARY_DIR}/include/c++/v1")
-    set(LIBCXX_GENERATED_MODULE_DIR "${CMAKE_BINARY_DIR}/modules/c++/v1")
   endif()
   set(LIBCXX_GENERATED_INCLUDE_TARGET_DIR "${LIBCXX_GENERATED_INCLUDE_DIR}")
   set(LIBCXX_INSTALL_LIBRARY_DIR lib${LIBCXX_LIBDIR_SUFFIX} CACHE STRING

Created using spr 1.3.6-beta.1
@arichardson arichardson changed the base branch from main to users/arichardson/spr/main.libc-stop-copying-headers-to-the-build-directory November 7, 2024 22:47
@arichardson arichardson requested a review from a team as a code owner November 7, 2024 22:47
@@ -1021,17 +1021,8 @@ set(files
configure_file("__config_site.in" "${LIBCXX_GENERATED_INCLUDE_TARGET_DIR}/__config_site" @ONLY)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ldionne Unless I am missing something it appears this file is unused. If this is correct I'll open a new PR to delete it and will rebase on top of that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't used yet.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@philnik777 Should be be modifying the C++03 CMakeLists.txt at all?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this change is fine. I'll have to modify this anyways and removing the copying now means I won't introduce it by accident again.

Created using spr 1.3.6-beta.1
Copy link
Member

@ldionne ldionne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great, this is a really nice cleanup. Thank you!

Let's just wait until we know what to do with the C++03 CMakeLists but I'm OK with merging this once that's settled, if CI is green.

libcxx/include/CMakeLists.txt Outdated Show resolved Hide resolved
@@ -1021,17 +1021,8 @@ set(files
configure_file("__config_site.in" "${LIBCXX_GENERATED_INCLUDE_TARGET_DIR}/__config_site" @ONLY)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@philnik777 Should be be modifying the C++03 CMakeLists.txt at all?

Created using spr 1.3.6-beta.1
@arichardson
Copy link
Member Author

Thanks for the review - will merge once #115387 has landed and CI is happy.

Created using spr 1.3.6-beta.1
@arichardson
Copy link
Member Author

It looks like the lldb test suite depends on these files being copied as well, so I've updated it to depend on the runtimes-test-depends target to ensure the libc++ headers have been installed to the fake prefix in the build dir first. Hardcoding these paths is unfortunate but I couldn't see an obviously better solution. This dates back to https://reviews.llvm.org/D133973

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. lldb
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants